Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Porting the Conv kernel from Tensorflow Lite to Tensorflow Lite Micro #27362

Merged
merged 7 commits into from
May 12, 2019
Merged

Porting the Conv kernel from Tensorflow Lite to Tensorflow Lite Micro #27362

merged 7 commits into from
May 12, 2019

Conversation

jenselofsson
Copy link
Contributor

This PR ports the Conv kernel in Tensorflow Lite to Tensorflow Lite Micro.

@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Apr 1, 2019
@gbaned gbaned added comp:lite TF Lite related issues size:L CL Change Size: Large labels Apr 1, 2019
@rthadur rthadur requested review from jdduke and lu-wang-g April 1, 2019 15:19
@jdduke
Copy link
Member

jdduke commented Apr 1, 2019

I believe we have some engineers already working on this kernel. @petewarden can offer guidance.

@jdduke jdduke requested a review from petewarden April 1, 2019 21:59
wangtz
wangtz reviewed Apr 2, 2019
View reviewed changes
Copy link
Member

@wangtz wangtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Thanks for your PR.
Please also add the corresponding entry in the BUILD file.

Here is my first round of code review, mainly focusing on the kernel implementation.

tensorflow/lite/experimental/micro/kernels/all_ops_resolver.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
tensorflow/lite/experimental/micro/kernels/conv.cc Outdated Show resolved Hide resolved
wangtz
wangtz previously approved these changes Apr 8, 2019
View reviewed changes
Copy link
Member

@wangtz wangtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sry for the delay. This looks great! Thanks!

Would love to sync with you on the next plan to avoid duplication of efforts. What are the operators missing in your use case that you would like the support next? Thanks

tensorflow/lite/experimental/micro/kernels/conv_test.cc Outdated
@@ -0,0 +1,329 @@
/* Copyright 2017 The TensorFlow Authors. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2019

@wangtz
Copy link
Member

wangtz commented Apr 8, 2019

Nits: next time, better not to use force push so that the comment history and diff can be preserved.

@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 8, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 8, 2019
@rthadur
Copy link
Contributor

rthadur commented Apr 8, 2019

@jenselo can you please address Ubuntu Sanity build failures.

@rthadur rthadur removed the ready to pull PR ready for merge process label Apr 8, 2019
petewarden
petewarden previously approved these changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@petewarden petewarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done by @wangtz, just formally approving.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Apr 8, 2019
@jenselofsson jenselofsson dismissed stale reviews from petewarden and wangtz via f9153a8 April 9, 2019 08:13
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Apr 9, 2019
@jenselofsson
Copy link
Contributor Author

@jenselo can you please address Ubuntu Sanity build failures.

Yes, I'm looking into it today.

wangtz
wangtz previously approved these changes Apr 29, 2019
View reviewed changes
Copy link
Member

@wangtz wangtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label Apr 29, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 29, 2019
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Apr 30, 2019
@gbaned
Copy link
Contributor

gbaned commented May 6, 2019

@jenselo could you please fix build errors. Thanks !

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes May 7, 2019
@gbaned gbaned requested a review from wangtz May 7, 2019 08:59
wangtz
wangtz previously approved these changes May 7, 2019
View reviewed changes
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer May 7, 2019
@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label May 7, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 7, 2019
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes May 7, 2019
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer May 7, 2019
@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label May 7, 2019
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels May 7, 2019
@gbaned gbaned added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels May 10, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 10, 2019
@tensorflow-copybara tensorflow-copybara merged commit 8e56e4a into tensorflow:master May 12, 2019
PR Queue automation moved this from Approved by Reviewer to Merged May 12, 2019
tensorflow-copybara pushed a commit that referenced this pull request May 12, 2019
@tensorflower-gardener
Merge pull request #27362 from jenselo:micro_port_conv
955de4b 
PiperOrigin-RevId: 247806095
@jenselofsson jenselofsson deleted the micro_port_conv branch May 13, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangtz wangtz wangtz approved these changes

@petewarden petewarden Awaiting requested review from petewarden

Assignees

@gbaned gbaned

Labels
cla: yes comp:lite TF Lite related issues ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jenselofsson @jdduke @wangtz @rthadur @gbaned @petewarden @googlebot @kokoro-team @tensorflow-copybara